Skip to content

reject NaN and infinity in double and float validators#384

Open
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:double-float-reject-nan
Open

reject NaN and infinity in double and float validators#384
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:double-float-reject-nan

Conversation

@sahvx655-wq
Copy link
Copy Markdown
Contributor

reading the number validators, validate of double and float passes the parsed value straight through processParsedValue without checking it is finite. NumberFormat recognises the locale NaN and infinity symbols, so validate("NaN"), validate("∞") and validate("-∞") return NaN/infinity and isValid returns true. a NaN that validates then silently defeats every isInRange/minValue/maxValue check, since NaN compares false against everything. reject non-finite results in both validators.

@garydgregory
Copy link
Copy Markdown
Member

@sahvx655-wq

Did you read the Javadoc?

This looks like you or your AI are imagining requirements that don't exist or that I can't find.

For example, "NaN" is a valid value to parse with a NumberFormat (which we use):

jshell> import java.text.*;
jshell> NumberFormat.getInstance().parse("NaN");
$3 ==> NaN

But it's not the same for Double.POSITIVE_INFINITY:

jshell> NumberFormat.getInstance().parse("" + Double.POSITIVE_INFINITY)
|  Exception java.text.ParseException: Unparseable number: "+Infinity"
|        at NumberFormat.parse (NumberFormat.java:434)
|        at (#2:1)

So there is a discrepancy...

Aside from the issue that the PR this may break existing applications, how do you propose an app use this class to validate, for example, config file values, or user input where in fact, a Double value like Double.NaN is expected?

FYI, I updated tests for edge case tests in FloatValidatorTest and DoubleValidatorTest to reflect the current code.

TY!

@sahvx655-wq
Copy link
Copy Markdown
Contributor Author

fair point. NaN is a legitimate Double value and NumberFormat parses it by design, so rejecting it here is a behaviour change that could break callers who genuinely want to validate it. my reasoning was that a NaN which passes then slips past every isInRange/minValue check, since NaN compares false against everything, but on reflection that guard belongs to the caller, not this class. and as you spotted the infinity handling is already inconsistent (NaN and the ∞ symbol parse, +Infinity doesn't), which isn't something to paper over in this PR. happy to close this out, your edge-case test updates already pin the current behaviour as the sensible default.

@garydgregory
Copy link
Copy Markdown
Member

Also note that NaN parsing behavior is different on Java 8 compared to newer versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants